[UIKit] Avoid useless measure invalidation propagation cycles#33459
Conversation
|
Hey there @@albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
I'm reviewing this PR and noticed the iOS CollectionView2 test failed. Is this a known flaky test, or could this be related to the measure invalidation optimization changes? |
|
It may have broken it, interesting, I'll look into it, thanks @kubaflo |
|
@albyrock87 what's the status of this PR? Do you think it is still needed? If so, can you please resolve conflicts? |
@kubaflo yes I think this is needed. |
|
|
320d826 to
81f7e87
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33459Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33459" |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@kubaflo it looks like iOS is not failing anymore (I simply rebased onto |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
🚦 Gate - Test Before and After Fix📊 Expand Full Gate —
|
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33459 | Add bool _measureInvalidatedPropagated flag to MauiView; reset in SizeThatFits/LayoutSubviews; guard in InvalidateMeasure |
MauiView.cs |
Original PR |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33459 | Add bool _measureInvalidatedPropagated to MauiView; guard both isPropagating paths; reset in SizeThatFits + LayoutSubviews |
MauiView.cs (17 additions) |
Simplest, closest to codebase style | |
| 1 | try-fix (opus) | Add bool _ancestorPropagationHandled to MauiView, only guard isPropagating=true path |
✅ PASS (31 passed, 0 failed) | MauiView.cs |
Narrower scope — only deduplicates ancestor relay calls, not originator calls |
| 2 | try-fix (sonnet) | [ThreadStatic] HashSet<IntPtr> visited set in ViewExtensions.InvalidateAncestorsMeasures walker |
✅ PASS (62 passed, 0 failed) | ViewExtensions.cs |
No per-view state; uses shared walker-level tracking; GCD cleanup |
| 3 | try-fix (codex) | Reuse existing NaN measure cache (_lastMeasureWidth/_lastMeasureHeight) as dedup signal — no new fields |
✅ PASS (31 passed, 0 failed) | MauiView.cs |
Clever reuse of existing state; slightly fragile — relies on InvalidateConstraintsCache NaN semantics |
| 4 | try-fix (gpt-5.4) | Layer.NeedsLayout() pre-check to stop propagation (no new fields) |
❌ BLOCKED (AOT launch failure — infrastructure issue, not code bug) | MauiView.cs, ViewExtensions.cs, MauiScrollView.cs |
App failed to launch in xharness; compile issue resolved but environment blocked testing |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | "NO NEW IDEAS" — all reasonable strategy categories covered |
Exhausted: Yes
Selected Fix: PR #33459 approach — _measureInvalidatedPropagated bool flag in MauiView
Reason:
- Simplest implementation (17 additions, 1 file)
- Most consistent with existing codebase style (follows pattern of other cached bool fields like
_parentHandlesSafeArea) - Guards BOTH paths (originator + ancestor relay) providing complete deduplication
- Clear, explicit lifecycle: reset at entry to
SizeThatFitsandLayoutSubviews - Attempt 1 (ancestor-only guard) is a subset of the PR's behavior; PR's approach is more comprehensive
- Attempt 2 (ThreadStatic HashSet) is functionally correct but introduces shared mutable state, GCD dependencies, and more complexity than needed
- Attempt 3 (NaN reuse) is clever but fragile — depends on
InvalidateConstraintsCachesemantics, harder to reason about
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | iOS-only perf optimization, 1 file, community PR |
| Gate | No tests detected in PR | |
| Try-Fix | ✅ COMPLETE | 4 attempts (3 passing, 1 blocked by infra); PR's fix selected as best |
| Report | ✅ COMPLETE |
Summary
PR #33459 adds a sensible and correct iOS-specific performance optimization to MauiView that prevents redundant measure invalidation propagation cycles. The approach is sound and passes all Layout device tests in independent exploration. However, the PR has two issues that should be addressed before merge:
-
No tests — Gate was skipped because no tests were included. This is a performance optimization, but at minimum a regression test or device test verifying that layouts with multiple rapidly-changing children still render correctly would increase confidence.
-
Stale comment — The comment
// If we're not propagating, then this view is the one triggering the invalidationis now misleading because the new_measureInvalidatedPropagatedguard applies to BOTHisPropagating=trueandisPropagating=false(whenHasFixedConstraints=false). The comment should be updated to reflect that the guard prevents redundant propagation in all cases. -
Merge conflicts — PR currently has merge conflicts (author rebased on Apr 9, 2026; CI re-triggered same day).
Root Cause
In iOS UIKit, MauiView.InvalidateMeasure() → ViewExtensions.InvalidateAncestorsMeasures() walks the entire ancestor view chain via a while loop calling InvalidateMeasure(isPropagating: true) on each ancestor. When multiple properties change in a binding context update (e.g., N Labels in a StackLayout all update their Text), this walk is triggered N times, redundantly calling SetNeedsLayout() on the same ancestor views and walking up the chain N times. After the first walk, UIKit already has all ancestors in its pending-layout set; subsequent walks are pure overhead.
This is the same deduplication that Android's requestLayout() provides natively — once a view has the "needs layout" flag, calling requestLayout() again is a no-op for propagation.
Fix Quality
The PR's fix is correct and is the best among all alternatives explored:
| Approach | Correctness | Simplicity | Style |
|---|---|---|---|
PR: _measureInvalidatedPropagated bool flag (both paths) |
✅ Correct | ✅ 17 lines, 1 file | ✅ Matches existing _parentHandlesSafeArea pattern |
Alt 1: _ancestorPropagationHandled (isPropagating=true only) |
✅ Correct | ✅ Similar size | |
| Alt 2: ThreadStatic HashSet in walker | ✅ Correct | ||
| Alt 3: NaN measure cache as dedup signal | ✅ Correct | InvalidateConstraintsCache semantics |
|
Alt 4: Layer.NeedsLayout() pre-check |
❓ Blocked | — | — |
Selected Fix: PR — The PR's implementation is the simplest, most readable, and most consistent with the codebase's existing patterns for cached boolean state.
Issues to address before merge:
- Fix the stale comment above the new guard (it says "If we're not propagating" but the guard applies to both cases)
- Add at minimum a basic layout device test to ensure no regression (e.g., verifying a StackLayout with multiple text-updating Labels renders correctly)
- Resolve merge conflicts
|
Failed UI tests are unrelated (Android). |
Description of Change
Invalidation propagation can be quite consuming especially when switching binding context and multiple properties change (i.e. having multiple labels inside a layout, all of them changing the text).
Ti PR avoids useless propagations (same behavior
requestLayoutintrinsically has on Android).